chore(examples): add pubsub example with production env by alrevuelta · Pull Request #1333 · logos-messaging/logos-delivery

alrevuelta · 2022-11-03T09:32:53Z

Simple pubsub example to showcase how nwaku can be used. Some notes:

Add a new example with two codes: publisher and subscriber.
Both publisher and subscriber connect to our fleet, discovering news peers and connecting to them.
Publisher and subscriber can run on different machines, and is not a requirements that they are discoverable.
Default pubsub topic is used.
Bare minimum code that a client can use to leverage waku network.

status-im-auto · 2022-11-03T09:41:18Z

Jenkins Builds

Click to see older builds (6)

❔	Commit	#️⃣	Finished (UTC)	Duration	Platform	Result
❌	`5aa334b`	#2	2022-11-03 09:41:18	~6 min	`macos`	📄`log`
✔️	`5aa334b`	#2	2022-11-03 09:52:29	~17 min	`linux`	📦`bin`

✔️	`1ade95a`	#1	2022-11-03 09:51:09	~17 min	`linux`	📦`bin`
✔️	`1ade95a`	#1	2022-11-03 09:52:29	~19 min	`macos`	📦`bin`

✔️	`32bb0c3`	#3	2022-11-04 08:31:15	~17 min	`linux`	📦`bin`
✔️	`32bb0c3`	#3	2022-11-04 08:35:08	~21 min	`macos`	📦`bin`

❔	Commit	#️⃣	Finished (UTC)	Duration	Platform	Result
✔️	`ac6efe9`	#4	2022-11-04 08:35:33	~17 min	`linux`	📦`bin`
✔️	`ac6efe9`	#4	2022-11-04 08:41:12	~22 min	`macos`	📦`bin`

✔️	`09e91c7`	#5	2022-11-04 12:57:12	~17 min	`linux`	📦`bin`
✔️	`09e91c7`	#5	2022-11-04 13:01:31	~21 min	`macos`	📦`bin`

LNSD

Please check my comments 🙂

I recommend reading more code within nwaku repository (production and test cases code). Many comments could be avoided by looking for usage examples within the repository.

LNSD · 2022-11-03T14:12:42Z

+
+    # Make sure it matches the publisher. Use default value
+    # see spec: https://rfc.vac.dev/spec/23/
+    let pubSubTopic = cast[PubsubTopic]("/waku/2/default-waku/proto")


Suggested change

let pubSubTopic = cast[PubsubTopic]("/waku/2/default-waku/proto")

let pubSubTopic = PubsubTopic("/waku/2/default-waku/proto")

Actually, this was a copy-paste from basic2 example, will update the example then.

Yeah, apologies. The examples are quite outdated (esp. in terms of Nim usage!) so they've been making for a bad example. :/

LNSD · 2022-11-03T14:14:17Z

+    let pubSubTopic = cast[PubsubTopic]("/waku/2/default-waku/proto")
+
+    # any content topic can be chosen. make sure it matches the publisher
+    let contentTopic = ContentTopic("/waku/2/pubsub-exampe/proto")


This content topic should follow the 23/WAKU2-TOPICS RFC in regards of the content topics: https://rfc.vac.dev/spec/23/#content-topics

What do you mean? I took the RFC as reference. app/version/name/enconding?

/{application-name}/{version-of-the-application}/{content-topic-name}/{encoding}

I'm also confused here as to why this wouldn't be following the pattern? If Waku is seen as its own application, which we often do for examples, this would be following the pattern imo.

The correct one for me would be something around the following:

application-name: examples or waku-examples (waku should not be a valid application name)

version-of-the-application: 1 (this is the first version of the application)

content-topic-name: relay-example or pubsub-example

encoding: proto

sure, `pubsub-example seems more accurate.

(waku should not be a valid application name)

what do you mean? do you want to enforce this?

speaking about this, with #1335 I have discovered that there are multiple content toppics that don't respect the spec, i.e. they have more fields. is this something that we want to enforce at some point?

We won't enforce any content topic formats. /waku is used widely as an application name already, particularly for bridging from waku v1 (in which /waku/1 is seen as the "application" and "version". An application is roughly defined as "something that defines its own payload format", but it's not possible to be precise here as content topics are not used in exactly the same way by all platforms, sometimes for good reason.

LNSD · 2022-11-03T14:14:40Z

+
+    proc handler(topic: PubsubTopic, data: seq[byte]) {.async, gcsafe.} =
+      let message = WakuMessage.init(data).value
+      let payload = cast[string](message.payload)


Suggested change

let payload = cast[string](message.payload)

let payload = $message.payload

note that in my case it should be string.fromBytes(message.payload) since as we haven't overloaded $ operator, $message.payload just prints the bytes and not the strings :)

LNSD · 2022-11-03T14:14:59Z

+    # any content topic can be chosen. make sure it matches the publisher
+    let contentTopic = ContentTopic("/waku/2/pubsub-exampe/proto")
+
+    proc handler(topic: PubsubTopic, data: seq[byte]) {.async, gcsafe.} =


Suggested change

proc handler(topic: PubsubTopic, data: seq[byte]) {.async, gcsafe.} =

proc handler(pubsubTopic: PubsubTopic, data: seq[byte]) {.async, gcsafe.} =

LNSD · 2022-11-03T14:18:07Z

+    while true:
+      let numConnectedPeers = node.peerManager.peerStore.connectionBook.book.values().countIt(it == Connected)
+      if numConnectedPeers >= 6:
+        notice "subscriber is ready", connectedPeers=numConnectedPeers, required=6
+        break
+      notice "waiting to be ready", connectedPeers=numConnectedPeers, required=6
+      await sleepAsync(5000)


I would rather not abuse of using while true: + sleepAsync(). Use chronos' setTimer like waku_metrics does: https://github.com/status-im/nwaku/blob/master/waku/v2/node/waku_metrics.nim#L42-L76

this loop breaks in 3-4 iterations once the minimum amount of peers are reached. I also want to block the execution here until these number of peers are reached. Using setTimer doesn't seem to be the best choice for here, but correct me if im wrong.

LNSD · 2022-11-03T14:39:12Z

+    notice "publisher service started"
+    while true:
+      let text = "hi there i'm a publisher"
+      let message = WakuMessage(payload: cast[seq[byte]](text), # content of the message


Use stew/byteutils:

Suggested change

let message = WakuMessage(payload: cast[seq[byte]](text), # content of the message

let message = WakuMessage(payload: toBytes(text), # content of the message

nice, will update basic2 example as well.

LNSD · 2022-11-03T14:40:02Z

+      notice "published message", text = text, timestamp = message.timestamp, psTopic = pubSubTopic, contentTopic = contentTopic
+      await sleepAsync(5000)
+
+discard setupAndPublish()


Idem. Don't discard futures. Use asyncSpawn.

LNSD · 2022-11-03T14:40:26Z

+
+    # Make sure it matches the publisher. Use default value
+    # see spec: https://rfc.vac.dev/spec/23/
+    let pubSubTopic = cast[PubsubTopic]("/waku/2/default-waku/proto")


Suggested change

let pubSubTopic = cast[PubsubTopic]("/waku/2/default-waku/proto")

let pubSubTopic = PubsubTopic("/waku/2/default-waku/proto")

same, copy-paste from basic2 example. ofc makes no sense

LNSD · 2022-11-03T14:41:05Z

+    let pubSubTopic = cast[PubsubTopic]("/waku/2/default-waku/proto")
+
+    # any content topic can be chosen
+    let contentTopic = ContentTopic("/waku/2/pubsub-exampe/proto")


Idem. This content topic is not following the 23/WAKU2-TOPICS

LNSD · 2022-11-03T14:42:33Z

+    node.wakuDiscv5 = WakuDiscoveryV5.new(
+        none(ValidIpAddress), none(Port), none(Port),
+        ip, Port(discv5Port), bootstrapNodes, false,
+        keys.PrivateKey(nodeKey.skkey), flags, [], node.rng)


Idem. "Magic numbers". Use key annotated arguments.

jm-clius

This is a great addition, thanks! Our examples have needed some TLC for a while. Some minor comments below. I'd also prefer keeping the number of make targets simple for now (make example2 should cut it).

jm-clius · 2022-11-03T15:26:49Z

+import
+  confutils,
+  std/[tables,sequtils],
+  chronicles,
+  chronicles/topics_registry,
+  chronos,
+  stew/shims/net,
+  libp2p/crypto/crypto,
+  eth/keys,
+  eth/p2p/discoveryv5/enr


What is the pattern here @LNSD ? 🤔 I usually have std/ first, then external imports alphabetically, then internal, relative imports from furthest removed to closest. (Not really established before, but happy to make this convention.) I thought it's similar here, but then I'm unsure about stew 🤷

jm-clius · 2022-11-03T15:35:44Z

@@ -0,0 +1,33 @@
+# basic2


Is it just me doing something dumb, or does Github not render markdown headers correctly if the first letter is not capitalised (which would be annoying)?

Github doesn't render them looks like :0

jm-clius · 2022-11-03T15:40:50Z

+    let contentTopic = ContentTopic("/waku/2/pubsub-exampe/proto")
+
+    proc handler(topic: PubsubTopic, data: seq[byte]) {.async, gcsafe.} =
+      let message = WakuMessage.init(data).value


Up to you, but I'm OK with an example crashing when a seriously unexpected condition arises. Could even consider using expect to crash with a message.

jm-clius · 2022-11-03T15:58:23Z

+    let pubSubTopic = cast[PubsubTopic]("/waku/2/default-waku/proto")
+
+    # any content topic can be chosen. make sure it matches the publisher
+    let contentTopic = ContentTopic("/waku/2/pubsub-exampe/proto")


We won't enforce any content topic formats. /waku is used widely as an application name already, particularly for bridging from waku v1 (in which /waku/1 is seen as the "application" and "version". An application is roughly defined as "something that defines its own payload format", but it's not possible to be precise here as content topics are not used in exactly the same way by all platforms, sometimes for good reason.

alrevuelta · 2022-11-04T08:18:44Z

@LNSD thanks for the comments! just fixed them!

LNSD

LGTM ✅

alrevuelta force-pushed the pubsub-example branch from 1ade95a to 5aa334b Compare November 3, 2022 09:34

alrevuelta requested review from LNSD, jm-clius and rymnc November 3, 2022 09:35

LNSD suggested changes Nov 3, 2022

View reviewed changes

jm-clius approved these changes Nov 3, 2022

View reviewed changes

LNSD self-requested a review November 3, 2022 23:30

alrevuelta force-pushed the pubsub-example branch from 32bb0c3 to ac6efe9 Compare November 4, 2022 08:18

LNSD approved these changes Nov 4, 2022

View reviewed changes

alrevuelta added 3 commits November 4, 2022 13:39

chore(examples): add pubsub example with production env

20160ed

chore(examples): fix comments 1/2

1225f7d

chore(examples): fix comments 2/2

09e91c7

alrevuelta force-pushed the pubsub-example branch from ac6efe9 to 09e91c7 Compare November 4, 2022 12:39

alrevuelta merged commit 1244342 into master Nov 4, 2022

alrevuelta deleted the pubsub-example branch November 4, 2022 12:40

alrevuelta mentioned this pull request Nov 14, 2022

docs: release v0.13.0 #1378

Merged

	let pubSubTopic = cast[PubsubTopic]("/waku/2/default-waku/proto")
	let pubSubTopic = PubsubTopic("/waku/2/default-waku/proto")

	let payload = cast[string](message.payload)
	let payload = $message.payload

	proc handler(topic: PubsubTopic, data: seq[byte]) {.async, gcsafe.} =
	proc handler(pubsubTopic: PubsubTopic, data: seq[byte]) {.async, gcsafe.} =

	let message = WakuMessage(payload: cast[seq[byte]](text), # content of the message
	let message = WakuMessage(payload: toBytes(text), # content of the message

Conversation

alrevuelta commented Nov 3, 2022

Uh oh!

status-im-auto commented Nov 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Jenkins Builds

Uh oh!

LNSD left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jm-clius left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alrevuelta commented Nov 4, 2022

Uh oh!

LNSD left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

status-im-auto commented Nov 3, 2022 •

edited

Loading

LNSD left a comment •

edited

Loading